Skip to content

[stable27] Revert change that made OC.Util.humanFileSize return decimal sizes instead of binary #40605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
susnux merged 2 commits into from
Sep 25, 2023
Merged

[stable27] Revert change that made OC.Util.humanFileSize return decimal sizes instead of binary #40605

susnux merged 2 commits into from
Sep 25, 2023

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Sep 24, 2023

Summary

Previously the OC.Util.humanFileSize was returning file sizes base 2, meaning 1024 bytes = 1 KiB, but the @nextcloud/files library had a regression that set the default to base 10. Meaning 1000 bytes = 1 KB.

This is fixed for current @nextcloud/libraries (3.0.0-beta.23) but for stable27 we need to fix this manually by wrapping the function.

Checklist

@susnux susnux added bug 3. to review Waiting for reviews regression labels Sep 24, 2023
@susnux susnux requested review from a team, Pytal, artonge, nfebe and szaimen and removed request for a team September 24, 2023 14:54
@susnux susnux added this to the Nextcloud 27.1.2 milestone Sep 24, 2023
@susnux susnux force-pushed the fix/format-file-sizes branch from 4539c7b to 44ca6bf Compare September 24, 2023 14:59
@susnux
fix: Revert change that made OC.Util.humanFileSize return base 10 i…
eb73c0b 
…nstead of base 2

Previously the `OC.Util.humanFileSize` was returning file sizes base 2, meaning 1024 bytes = 1 KiB, but the `@nextcloud/files` library had a regression that set the default to base 10. Meaning 1000 bytes = 1 KB.

This is fixed for current `@nextcloud/libraries` but for stable27 we need to fix this manually by wrapping the function.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux
chore: Compile assets
b7a939d 
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/format-file-sizes branch from 44ca6bf to b7a939d Compare September 24, 2023 15:13
@susnux
Copy link
Contributor Author

susnux commented Sep 24, 2023

PS: This is just a bugfix so I will not touch the issue that we using binary sizes but showing decimal units. This just restores the previous behavior.

@susnux susnux changed the title fix: Revert change that made OC.Util.humanFileSize return decimal sizes instead of binary [stable27] Revert change that made OC.Util.humanFileSize return decimal sizes instead of binary Sep 24, 2023
@susnux
Copy link
Contributor Author

susnux commented Sep 24, 2023

drone unrelated

@susnux susnux linked an issue Sep 25, 2023 that may be closed by this pull request
wrong user quota in the user administration #38874
Closed
8 tasks
@susnux susnux merged commit 049aa5d into stable27 Sep 25, 2023
@susnux susnux deleted the fix/format-file-sizes branch September 25, 2023 13:27
@susnux susnux mentioned this pull request Sep 25, 2023
Closed
8 tasks
@come-nc
Copy link
Contributor

come-nc commented Sep 26, 2023

I think this broke node tests: https://github.com/nextcloud/server/actions/runs/6301358504/job/17107304772

  OCA.Files.FileList tests
    Removing files from the list
      ✗ Removes file from list when calling remove() and updates summary
	Expected '69 KB' to equal '70 KB'.
	    at Object.<anonymous> (apps/files/tests/js/filelistSpec.js:542:46)

    Deleting files
      ✗ calls delete.php, removes the deleted entries and updates summary
	Expected '57 KB' to equal '58 KB'.
	    at apps/files/tests/js/filelistSpec.js:612:47

    List rendering
      ✗ updates summary using the file sizes
	Expected '69 KB' to equal '70 KB'.
	    at Object.<anonymous> (apps/files/tests/js/filelistSpec.js:1141:46)


  OCA.Files.FileSummary tests
    ✗ renders summary as text
	Expected '250 KB' to equal '256 KB'.
	    at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:47:47)

    ✗ increases summary when adding files
	Expected '500 KB' to equal '512 KB'.
	    at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:71:47)

    ✗ decreases summary when removing files
	Expected '125 KB' to equal '128 KB'.
	    at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:89:47)

    ✗ renders filtered summary as text
	Expected '250 KB' to equal '256 KB'.
	    at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:107:47)

    ✗ increases filtered summary when adding files
	Expected '500 KB' to equal '512 KB'.
	    at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:136:47)

    ✗ decreases filtered summary when removing files
	Expected '125 KB' to equal '128 KB'.
	    at Object.<anonymous> (apps/files/tests/js/filesummarySpec.js:158:47)

@susnux susnux mentioned this pull request Sep 26, 2023
Merged
4 tasks
@blizzz blizzz mentioned this pull request Sep 26, 2023
Merged
@luzfcb
Copy link

luzfcb commented Sep 28, 2023

@susnux @artonge Do you have any idea if the problem that this PR fixes could possibly also be the cause of nextcloud/android#11974 ?

@come-nc
Copy link
Contributor

come-nc commented Oct 2, 2023

@susnux @artonge Do you have any idea if the problem that this PR fixes could possibly also be the cause of nextcloud/android#11974 ?

No, this PR only touches the web UI

@susnux susnux mentioned this pull request Oct 5, 2023
Merged
4 tasks
@Jerome-Herbinet
Copy link
Member

Jerome-Herbinet commented Oct 11, 2023 •
edited
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artonge artonge artonge approved these changes

@nfebe nfebe Awaiting requested review from nfebe nfebe was automatically assigned from nextcloud/server-frontend

@Pytal Pytal Awaiting requested review from Pytal Pytal was automatically assigned from nextcloud/server-frontend

@szaimen szaimen Awaiting requested review from szaimen szaimen was automatically assigned from nextcloud/server-frontend

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug regression

Projects

None yet

Milestone

Nextcloud 27.1.2

Development

Successfully merging this pull request may close these issues.

wrong user quota in the user administration

6 participants

@susnux @come-nc @luzfcb @Jerome-Herbinet @artonge